Conversation
MOSU-153 refactor: 신청 조회시 신청 상태 구조 변경
MOSU-184 refactor: notify 과정 중 에러 발생 시 discord 알림 전송 구현
…le for starvation cleanup
…ent timestamp for createdAt and updatedAt
…rameterJdbcTemplate for batch deletion
…o feature/cron
…onFailureLogDomainArchiver
Quartz 를 통한 Application내에 실패한 객체를 Failure Log 로 이동 및 일정 주기에 따라서 삭제 하도록 구성
Fix#187: Cross Domain 쿠키 설정 변경
Fix: 쿠키 도메인 설정 주석 처리
…c for user and exam applications
…dling with exam application integration
…unt log integration
…irtual account creation
MOSU-216 feat: 카카오 심사 성공에 따른 필드 추가
…o feat/guest-application
…lAccountProcessor
비회원 신청 및 가상 계좌 신청에 따른 시험 인원 수 갱신
MOSU-205 feature: 관리자 시험 마감 기능 구현
MOSU-218 내 문의글 조회 내역 기능 구현
…Id to handle null values
…h support in self-depoly.yaml
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @polyglot-k, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
My primary goal with this pull request was to establish a clear separation between our production and test server pipelines, enabling more controlled and reliable deployments. Beyond that, I've undertaken a significant architectural overhaul, refactoring numerous core modules to adopt a more robust, event-driven design. This includes a complete rewrite of key functionalities like payment processing, application submissions, and refund management, alongside the introduction of several new features to enhance the platform's capabilities.
Highlights
- Deployment Strategy: I've implemented a robust Blue/Green deployment strategy using Docker Compose and Caddy, enabling seamless transitions between production and test environments with minimal downtime.
- Application Packaging and Runtime: I've migrated the application's packaging from JAR to WAR and updated the base JDK to Amazon Corretto, aligning with modern deployment practices and improving compatibility with servlet containers.
- Transactional Event-Driven Architecture: I've introduced a comprehensive transactional event-driven architecture across core business domains like payments, applications, refunds, inquiries, and user profiles, significantly enhancing system reliability and maintainability.
- Real-time Quota Management with Caching: I've integrated Redis-based atomic caching for managing exam quotas, ensuring real-time accuracy and preventing over-subscription, complete with robust recovery mechanisms.
- New Feature Development: I've developed a suite of new features, including extensive admin functionalities, event and form management, a recommendation system, virtual account payment processing, and a sophisticated notification system for various user interactions.
- Enhanced Testing Infrastructure: I've incorporated Testcontainers into the build process, which will greatly improve the reliability and consistency of our integration tests by providing disposable, real service containers.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a significant refactoring to separate production and test pipelines, including a blue-green deployment strategy. The changes are extensive, touching build configurations, deployment scripts, and a major overhaul of application services. The refactoring of services into smaller, more focused components is a great improvement for maintainability. However, there are several critical and high-severity issues that need to be addressed, such as a build-breaking typo in build.gradle, risky in-place file modifications in the deployment script, and potential data integrity issues in the OAuth user handling logic. I've provided detailed comments and suggestions for these points.
| testImplementation 'org.springframework.boot:spring-boot-testcontainers:3.3.5' | ||
| testImplementation 'org.testcontainers:testcontainers:1.19.3' | ||
| testImplementation 'org.testcontainers:junit-jupiter:1.19.3' | ||
| testImplementation 'org.testcoscntainers:mysql:1.20.0' |
There was a problem hiding this comment.
There is a typo in the Testcontainers dependency. It should be org.testcontainers instead of org.testcoscntainers. This will cause a build failure.
testImplementation 'org.testcontainers:mysql:1.20.0'
|
|
||
| echo "✅ 새 컨테이너 헬스 체크 성공." | ||
|
|
||
| sudo sed -i -E "s|(reverse_proxy /api/v1/\* localhost:)[0-9]+|\1${TARGET_PORT}|" /etc/caddy/Caddyfile |
There was a problem hiding this comment.
Modifying the Caddyfile in-place using sed is risky. If the file format changes, this command might fail silently or corrupt the configuration. A more robust approach would be to use a template for your Caddyfile and generate the final configuration with the correct port before reloading Caddy. You can use environment variables directly in Caddyfiles with the env global option.
| final UserJpaEntity newUser = UserJpaEntity.builder() | ||
| .loginId(info.email()) | ||
| .gender(Gender.MALE) | ||
| .name(info.name()) | ||
| .password("") | ||
| .birth(LocalDate.now()) | ||
| .userRole(UserRole.ROLE_USER) | ||
| //TODO kakao 정보 null일 경우 후처리 필요, ServiceTerm 승인 시 처리 구현 | ||
| .loginId(info.email() != null ? info.email() : "NA") | ||
| .gender(info.gender() != null ? info.gender() : Gender.MALE) | ||
| .name(info.name() != null ? info.name() : "NA") | ||
| .birth(info.birthDay() != null ? info.birthDay() | ||
| : LocalDate.EPOCH) | ||
| .phoneNumber(info.phoneNumber() != null ? info.phoneNumber() | ||
| : "010-0000-0000") | ||
| .userRole(UserRole.ROLE_PENDING) | ||
| .provider(AuthProvider.KAKAO) | ||
| .agreedToTermsOfService(true) | ||
| .agreedToPrivacyPolicy(true) | ||
| .agreedToMarketing(false) | ||
| .build(); |
There was a problem hiding this comment.
Using placeholder values like "NA" or LocalDate.EPOCH for user information that might be missing from the OAuth provider can lead to data integrity issues and unexpected behavior later. It would be more robust to either:
- Throw an exception if essential information is missing, forcing the user to provide it.
- Redirect the user to a profile completion page where they can fill in the missing details.
The TODO comment indicates you are aware of this, but this is a significant issue that should be addressed.
| ADD ${JAR_FILE} app.jar | ||
| ENTRYPOINT ["java", "-Duser.timezone=GMT+9", "-Djava.security.egd=file:/dev/./urandom", "-jar", "/app.jar"] No newline at end of file | ||
| FROM amazoncorretto:21 | ||
| COPY build/libs/*SNAPSHOT.war app.war |
There was a problem hiding this comment.
The pattern *SNAPSHOT.war is brittle as it will fail if you build a release version (which typically doesn't contain 'SNAPSHOT' in its name). Consider using a more generic pattern like *.war if you are sure there will be only one WAR file. An even more robust approach would be to rename the artifact to a fixed name (e.g., app.war) in your build.gradle file.
COPY build/libs/*.war app.war
|
|
||
| dependencies { | ||
|
|
||
| implementation fileTree(dir: 'libs', include: ['*.jar']) |
There was a problem hiding this comment.
Including JARs from a local libs directory is generally discouraged. It makes dependency management harder and the build less reproducible. It's better to publish these artifacts to a Maven repository (like Nexus, Artifactory, or even a local one) and declare them as regular dependencies.
| public void deleteByBannerId(Long bannerId) { | ||
| try { | ||
| bannerJpaRepository.deleteById(bannerId); | ||
| } catch (Exception e) { | ||
| throw new CustomRuntimeException(ErrorCode.BANNER_DELETE_FAILURE); | ||
| } | ||
| } |
There was a problem hiding this comment.
Catching a generic Exception is a bad practice as it can hide underlying issues. deleteById does not throw an exception if the entity is not found. It's better to check for the existence of the banner first and throw a specific exception if it's not found. This provides clearer error handling.
public void deleteByBannerId(Long bannerId) {
if (!bannerJpaRepository.existsById(bannerId)) {
throw new CustomRuntimeException(ErrorCode.BANNER_NOT_FOUND);
}
bannerJpaRepository.deleteById(bannerId);
}| public void examDateNotPassed(List<ExamJpaEntity> exams) { | ||
| boolean hasPassedExam = false; | ||
|
|
||
| for (ExamJpaEntity exam : exams) { | ||
| if (exam.getDeadlineTime().isBefore(LocalDateTime.now())) { | ||
| exam.close(); | ||
| hasPassedExam = true; | ||
| } | ||
| } | ||
|
|
||
| if (hasPassedExam) { | ||
| throw new CustomRuntimeException(ErrorCode.EXAM_DATE_PASSED); | ||
| } | ||
| } |
There was a problem hiding this comment.
This validation method has a side effect of modifying the state of ExamJpaEntity by calling exam.close(). Validators should be idempotent and free of side effects. The state modification should be handled in a service layer method after validation has passed. This separation makes the code easier to understand and test.
5 participants
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타